-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Axial to planar gradiometer transformation #13196
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
This is based on the channel positions and orientations provided by fieldtrip: https://github.com/fieldtrip/fieldtrip/blob/master/template/gradiometer/ctf275.mat
This is based on the channel positions and orientations provided by fieldtrip: https://github.com/fieldtrip/fieldtrip/blob/master/template/gradiometer/neuromag306.mat
|
Hello! 👋 Thanks for opening your first pull request here! ❤️ We will try to get back to you soon. 🚴 |
That way we can parse the txt files and select the channel type we want to interpolate to. For example we do not want to interpolate to the ch_type = 'ref' for the CTF.
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
|
Looks like you're making some progress, let me know when you'd like some feedback! |
|
Hi @larsoner! After a while, I finally found some time :) I think this is a good moment for some feedback. Issues / Questions I encountered: My initial understanding was that when we interpolate from CTF to Neuromag, the ch_type should already be known. This is because, CTF systems have reference sensors that should not be interpolated and Neuromag systems have gradiometers and magnetometers, so we need to know the type of each channel. That's why I added the ch_type parameter in the .txt montage files — to explicitly specify this. 2. Scope of interpolation: only gradiometers? Based on this, I thought I would input in 3. Limitations of make_dig_montage From the documentation of I understood that for my setup (no fiducials) I should use a custom montage. However: running 4. Custom _meg() function and related problems But now I run into new problems. When I run
5. I added _meg() inside _standard_montage_utils but am I allowed to change such private function ? 6. I am trying to follow how interpolate_to() code style and structure is already built. However, this forces us to diverge a bit from the standard One idea I had would be to encapsulate some logic in a new function |
Added new montage data files for CTF151, CTF275, and Neuromag306 systems in CSV format to mne/channels/data/montages/. These files provide sensor location and orientation information.
Introduces the read_meg_montage function to load canonical MEG sensor positions and orientations from CSV files for supported systems ('neuromag', 'ctf151', 'ctf275'). This utility constructs an Info object with sensor metadata for use in field interpolation.
for more information, see https://pre-commit.ci
This comment was marked as resolved.
This comment was marked as resolved.
|
I had forgotten some old code in this PR. I reverted all the commits that had this code. Now let's wait and see if the tests will pass ;) |
Reorganized the code to separate EEG and MEG system transformation sections.
mne/channels/montage.py
Outdated
| # TODO: Refactor not to use Pandas (should be able to parse without it) | ||
| df = pd.read_csv(csv_file) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pandas is an optional dependency of MNE and not everyone will have it. Is it easy enough to just parse the CSV directly / manually? Usually we try to do that if it's not too much work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now it is parsed manually
| if is_eeg_interpolation: | ||
| # Check that the method option is valid. | ||
| _validate_type(sensors, DigMontage, "sensors") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes here are mostly whitespace and nest things in conditionals. Rather than doing this, would it be possible to do something like
def interpolate_to(...)
...
if is_eeg_interpolation:
self._interpolate_to_eeg(...) # which contains most of the old code, unmodified
if is_meg_interpolation:
self._interpolate_to_meg(...) # which contains most of the new code
This separates out the functionality a bit in a more readable way, and improves git blame
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved EEG and MEG interpolation code from InterpolationMixin in channels.py to dedicated helper functions (_interpolate_to_eeg and _interpolate_to_meg) in interpolation.py.
|
@contsili do you have time to work on this? If it would help I could push some commits as well |
I was on holidays. I will work on it in the next two weeks and then you can take over to do the final adjustments if necessary |
for more information, see https://pre-commit.ci
Moved EEG and MEG interpolation code from InterpolationMixin in channels.py to dedicated helper functions (_interpolate_to_eeg and _interpolate_to_meg) in interpolation.py.
larsoner
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple of preliminary comments -- can you see if you can get CIs happy?
| def interpolate_to(self, sensors, origin="auto", method="spline", reg=0.0): | ||
| """Interpolate EEG data onto a new montage. | ||
| def interpolate_to( | ||
| self, sensors, origin="auto", method="MNE", mode="accurate", reg=0.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't change the method to "MNE" for EEG without a deprecation cycle. Let's make the default method=None which means under the hood (and in docs) "MNE for MEG sensors and spline for EEG".
| inst : instance of Raw, Epochs, or Evoked | ||
| A new instance with interpolated data. | ||
| """ | ||
| from .._fiff.meas_info import create_info |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CIs complain that some imports are nested that should not be etc.
Reference issue (if any)
Fixes #9609 .
What does this implement/fix?
Additional information
The channel positions and orientations are adopted from fieldtrip: https://github.com/fieldtrip/fieldtrip/blob/master/template/gradiometer
I am not sure if for the interpolation we need the coil positions and orientations. For clarity: a gradiometer is ONE channel but has TWO coils. A magnetometer is ONE channel and has ONE coil.
As an expectation I have that I will plot ERPs in the ctf and neuromag format. Also I want to create topoplots like: https://www.fieldtriptoolbox.org/assets/img/tutorial/eventrelatedaveraging/figure8.png